Skip to content

Remove ToggleInstructionStepModeCommand from Run Menu and Main Toolbar. #1143

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

raghucssit
Copy link
Contributor

As ToggleInstructionStepModeCommand is contributed in too many places. We can remove it from Main Toolbar and Run Command.

see #1142

@raghucssit raghucssit force-pushed the tism_cmd_fix_1142 branch 2 times, most recently from 1f55afa to a2afce3 Compare April 14, 2025 11:26
As ToggleInstructionStepModeCommand is contributed in too many places.
We can remove it from Main Toolbar and Run Command.

see eclipse-cdt#1142
Copy link

Test Results

   636 files  ± 0     636 suites  ±0   36m 34s ⏱️ +4s
11 439 tests ± 0  11 295 ✅ ± 0  144 💤 ±0  0 ❌ ±0 
11 442 runs   - 12  11 300 ✅  - 12  142 💤 ±0  0 ❌ ±0 

Results for commit a926d7f. ± Comparison against base commit 04105c2.

@iloveeclipse
Copy link
Contributor

@jonahgraham : a review is welcome.

@raghucssit
Copy link
Contributor Author

@jonahgraham The motivation for this fix is, " If property tester which decides the command has to the visible or not is not loaded then command is visible by default". Once the Property Tester is loaded by any dependents on the platform then it checks with the property tester and hides it. This is a bug/limitation in the eclipse platform.
Here is the detailed discussion:
eclipse-platform/eclipse.platform.ui#2900
As ToggleInstructionStepModeCommand is contributed to main toolbar, it is by default is visible even without any context.
So I have decided to remove it from there.
And it continues to be available at 'Debug View' toolbar.
Please check if we can remove this from main toolbar.

@jonahgraham
Copy link
Member

@jonahgraham : a review is welcome.

😢 I'm sorry that this slipped through the cracks - I was having extended down time when this came in.

@jonahgraham
Copy link
Member

@Kummallinen / @Torbjorn-Svensson (or any other CDT committer, especially one that has a CDT based product):

WDYT about removing instruction stepping mode from the main toolbar? Do your products rely/expect this? If so, can we put the onus on the product integrator to add this button back in for their use cases?

@iloveeclipse
Copy link
Contributor

So far no feedback. Can we merge and see if anyone complains? I guess not :-)

@Kummallinen
Copy link
Contributor

Sorry only just saw the notification for this.

For embedded development this is commonly used so would annoy our users if it were removed. While we can put it back for our products users of Embedded CDT would likely miss it & users of multiple plugins sets may end up with duplicates if vendors need to add this back in themselves

@iloveeclipse
Copy link
Contributor

The original issue is that the button is shown in non-CDT context as long as CDT is not loaded. This is a limitation of an extension point because it has to use tester classes to check the button / CDT debugging state, and we don't want CDT to be loaded if it is not used.

What about following options:

  1. Declare an activity that is disabled by default & bound that to the button. Whoever needs that can enable activity (either by product customization or via Preferences->Capabilities).
  2. Declare an activity that is disabled by default & bound that to the button. Additionally, add code to the corresponding bundle activator that would enable the activity on bundle startup and disable on shutdown.
  3. Declare an activity that is disabled by default & bound that to the button. Additionally, enable the activity first time CDT debugger starts (and do not disable it later).
  4. Any other ideas ? ...

WDYT?

@ruspl-afed
Copy link
Member

What if we add an "id" attribute for these contributions to let interested parties filter them out with E3 activity?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants